Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Run gen-server tests with Postgres #1336

Merged
merged 9 commits into from
Dec 15, 2024
Merged

CI: Run gen-server tests with Postgres #1336

merged 9 commits into from
Dec 15, 2024

Conversation

SleepyLeslie
Copy link
Collaborator

@SleepyLeslie SleepyLeslie commented Dec 6, 2024

Context

This is the successor of PR #1278.

Proposed solution

Related issues

N/A

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

fflorent and others added 6 commits December 2, 2024 00:47
Context: some server tests succeeded when using sqlite but failed with
postgresql. This commit runs the same tests using a postgresql database
to ensure both database types are supported for these tests.
@georgegevoian georgegevoian self-requested a review December 9, 2024 14:31
package.json Outdated Show resolved Hide resolved
app/server/lib/initialDocSql.ts Outdated Show resolved Hide resolved
Comment on lines 101 to 118
- name: Run gen-server tests with minio and redis
if: contains(matrix.tests, ':gen-server-')
run: |
PGPASSWORD=db_password psql -h localhost -U db_user -w db_name -c "SHOW ALL;" | grep ' jit '
export TEST_SPLITS=$(echo $TESTS | sed "s/.*:gen-server-\([^:]*\).*/\1/")
yarn run test:gen-server
TYPEORM_TYPE=postgres TYPEORM_HOST=localhost TYPEORM_DATABASE=db_name TYPEORM_USERNAME=db_user TYPEORM_PASSWORD=db_password yarn run test:gen-server
env:
MOCHA_WEBDRIVER_HEADLESS: 1
TESTS: ${{ matrix.tests }}
GRIST_DOCS_MINIO_ACCESS_KEY: administrator
GRIST_DOCS_MINIO_SECRET_KEY: administrator
TEST_REDIS_URL: "redis://localhost/11"
GRIST_DOCS_MINIO_USE_SSL: 0
GRIST_DOCS_MINIO_ENDPOINT: localhost
GRIST_DOCS_MINIO_PORT: 9000
GRIST_DOCS_MINIO_BUCKET: grist-docs-test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about splitting this up into two steps (e.g. Run gen-server tests with postgres and Run gen-server tests with sqlite)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it logically makes more sense to split them, but I put them together to avoid excessive duplication in the env section. Do you feel strongly about it?

Copy link
Collaborator

@fflorent fflorent Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would yaml's anchor / alias solve your issue? This resource help understanding the concept: https://www.educative.io/blog/advanced-yaml-syntax-cheatsheet

      - name: Run gen-server tests with minio and redis with sqlite
        if: contains(matrix.tests, ':gen-server-')
        run: |
          export TEST_SPLITS=$(echo $TESTS | sed "s/.*:gen-server-\([^:]*\).*/\1/")
          yarn run test:gen-server
        env: &gen-server-env-vars
          MOCHA_WEBDRIVER_HEADLESS: 1
          TESTS: ${{ matrix.tests }}
          GRIST_DOCS_MINIO_ACCESS_KEY: administrator
          GRIST_DOCS_MINIO_SECRET_KEY: administrator
          TEST_REDIS_URL: "redis://localhost/11"
          GRIST_DOCS_MINIO_USE_SSL: 0
          GRIST_DOCS_MINIO_ENDPOINT: localhost
          GRIST_DOCS_MINIO_PORT: 9000
          GRIST_DOCS_MINIO_BUCKET: grist-docs-test

      - name: Run gen-server tests with minio and redis with postgresql
        if: contains(matrix.tests, ':gen-server-')
        run: |
          PGPASSWORD=db_password psql -h localhost -U db_user -w db_name -c "SHOW ALL;" | grep ' jit '
          export TEST_SPLITS=$(echo $TESTS | sed "s/.*:gen-server-\([^:]*\).*/\1/")
          yarn run test:gen-server
        env: 
          <<: *gen-server-env-vars
          TYPEORM_TYPE: postgres
          TYPEORM_HOST: localhost
          TYPEORM_DATABASE: db_name
          TYPEORM_USERNAME: db_user
          TYPEORM_PASSWORD: db_password
          # Also change the TEST_REDIS_URL value?

And you might want to factorize a bit more… or not… Sometimes we have to choose a tradeoff between readability and factorization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fflorent Sadly anchors are not supported by GH actions: actions/runner#1182

Co-authored-by: George Gevoian <[email protected]>
- name: Run gen-server tests with minio and redis
if: contains(matrix.tests, ':gen-server-')
run: |
PGPASSWORD=db_password psql -h localhost -U db_user -w db_name -c "SHOW ALL;" | grep ' jit '
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this? Looks like a print for a test, I just want to be sure of that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An extra bit of info to confirm JIT is turned off successfully. For me it was useful for debugging CI problems, and I just kept it.

Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SleepyLeslie!

POSTGRES_USER: db_user
POSTGRES_PASSWORD: db_password
POSTGRES_DB: db_name
# JIT is enabled by default since Postgres 17 and has a huge negative impact on performance,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# JIT is enabled by default since Postgres 17 and has a huge negative impact on performance,
# JIT is enabled by default since Postgres 17 and has a huge negative impact on tests performance,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It affects not only test performance. See https://support.getgrist.com/self-managed/#what-is-a-home-database

Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SleepyLeslie

@SleepyLeslie SleepyLeslie merged commit a4196a2 into main Dec 15, 2024
12 checks passed
@SleepyLeslie SleepyLeslie deleted the add-postgresql-ci branch December 15, 2024 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants